-
Notifications
You must be signed in to change notification settings - Fork 411
fix(electrum): fix stale anchor hash on reorg #2011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(electrum): fix stale anchor hash on reorg #2011
Conversation
5bcc4f4 to
c8a3326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c8a3326
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this looks like a solid simplification and likely fixes the problem in the PR description. Can we add a test that triggers a reorg after fetch_tip_and_latest_blocks to confirm?
While reviewing, I noticed the chain-update logic is a bit convoluted, which makes this (and other bdk_electrum changes) tricky to follow:
block_header_cacheis keyed by height (not block hash), so it’s unclear how we’re ensuring the right header.- The
!validblock section updatesblock_header_cacheon reorgs afterfetch_tip_and_latest_blocks, meaning headers from different chains can co-exist. Anchors frombatch_fetch_anchorsmay also not match the same chain. - In
chain_update, we use blockhashes from anchors to craft theCheckPointupdate. Because inconsistencies are possible, we also pass inlastest_blocks, assuming reorgs aren’t deeper thanCHAIN_SUFFIX_LENGTH.
I suggest we merge this PR ASAP (since it’s an important fix).
Then, in a follow-up PR, we could simplify the codebase:
- Key
block_header_cacheby blockhash instead of height. Fetch by height through theCheckPointchain. - Remove the
!validblock section, since headers will always match the chain. Reorgs will naturally invalidate anchors and re-fetch them. - Let
fetch_tip_and_latest_blocks()just fetch blocks. Move chain-update logic intochain_update(), updated to takeheights_we_are_interested_ininstead of anchors. Callchain_update()afterbatch_fetch_anchors.
This may slightly slow down bdk_electrum (extra header lookups via CheckPoint), but IO is the real bottleneck. Once the skip-list is in, CheckPoint lookups will be fast.
On second though... maybe we should only do bug fixes and focus effort in bdk_electrum_streaming?
f5301df to
5499225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just a few nits in terms of documenting the test and unnecessary calls.
5499225 to
fdec863
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fdec863
Description
Fixes an issue in
batch_fetch_anchors()inbdk_electrumwhere, if a stale block header was detected and a fresh header was fetched, the confirmation anchor inserted intoanchor_cachestill used the hash from the original stale header instead of the new one.Changelog notice
batch_fetch_anchors()no longer uses a potentially stale hash as the anchor.Checklists
All Submissions:
Bugfixes: